Support client_trusts_lsp on LSPS2 #3838
Conversation
|
👋 I see @TheBlueMatt was un-assigned. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3838 +/- ##
==========================================
+ Coverage 88.76% 88.82% +0.05%
==========================================
Files 176 176
Lines 129345 129618 +273
Branches 129345 129618 +273
==========================================
+ Hits 114812 115127 +315
+ Misses 11925 11881 -44
- Partials 2608 2610 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
2 times, most recently
from
5d8508d to
a038ab6
Compare
|
A few extra concerns: HTLCs routed over 0-conf channels might hit CLTV timeouts if the channel should be confirmed but isn’t yet, so if the user takes forever to claim the payment, then there could be some trouble there. Also, I'm not sure if it would be better to have new possible states to explicitly model the idea of having sent the channel_ready but the funding_tx is not broadcasted yet. Also sharing the trust_model state between 4 of the 5 possible states is not something I'm convinced. I have a few different options for this, but I'm open to comments and suggestions |
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @tnull! This PR has been waiting for your review. |
There was a problem hiding this comment.
Thanks for looking into this.
This generally makes sense to me, although we should really work out how we'd deal with operational channels for which we withhold the funding transaction broadcast.
HTLCs routed over 0-conf channels might hit CLTV timeouts if the channel should be > confirmed but isn’t yet, so if the user takes forever to claim the payment, then there
could be some trouble there.
Yes. IMO this means that we need to introduce proper (read: clean API, and tested) support for 'hosted channels', i.e., channels that are operational even though the funding transaction hasn't been confirmed yet. Not sure if @TheBlueMatt has an opinion here?
Also, I'm not sure if it would be better to have new possible states to explicitly model > the idea of having sent the channel_ready but the funding_tx is not broadcasted yet. > Also sharing the trust_model state between 4 of the 5 possible states is not > something I'm convinced. I have a few different options for this, but I'm open to > comments and suggestions
Yeah, as mentioned in the comments, it would def. be preferable if we could avoid the many clones. Also it's overall a lot of boilerplate for just three fields handed through, maybe there is a simpler approach?
| @@ -11,6 +11,7 @@ | |||
|
|
|||
| use alloc::string::{String, ToString}; | |||
| use alloc::vec::Vec; | |||
| use bitcoin::Transaction; | |||
There was a problem hiding this comment.
nit: Let's move this down to the other bitcoin types.
|
|
||
| fn new(client_trusts_lsp: bool) -> Self { | ||
| if client_trusts_lsp { | ||
| return TrustModel::ClientTrustsLsp { |
There was a problem hiding this comment.
nit: Please avoid these explicit returns here.
| if client_trusts_lsp { | ||
| return TrustModel::ClientTrustsLsp { | ||
| funding_tx_broadcast_safe: false, | ||
| htlc_claimed: false, |
There was a problem hiding this comment.
nit: Maybe payment_claimed to align with the event type?
| let mut payment_queue = core::mem::take(payment_queue); | ||
| payment_queue.add_htlc(htlc); | ||
| *self = OutboundJITChannelState::PendingChannelOpen { | ||
| payment_queue, | ||
| opening_fee_msat: *opening_fee_msat, | ||
| trust_model: trust_model.clone(), |
There was a problem hiding this comment.
Would be good if we could find a way to avoid these clones. Given that these are just two bools and the transaction option, I also wonder if it's indeed worth all the boilerplate, or if it might suffice to have these fields live on the state/channel objects directly.
| Ok(()) | ||
| } | ||
|
|
||
| /// Called by ldk-node when the funding transaction is safe to broadcast. |
There was a problem hiding this comment.
Note that in this context we don't know where this will be used, so we shouldn't assume LDK Node is the only consumer of this API.
lightning/src/ln/channelmanager.rs
Outdated
| /// broadcast it manually. | ||
| /// | ||
| /// Used in LSPS2 on a client_trusts_lsp model | ||
| CheckedManualBroadcast(Transaction), |
There was a problem hiding this comment.
nit: Let's move to the second positition here and elsewhere.
lightning/src/ln/channelmanager.rs
Outdated
| /// # Warning | ||
| /// Improper use of this method could lead to channel state inconsistencies. | ||
| /// Ensure the transaction being broadcast is valid and expected by LDK. | ||
| pub fn unsafe_broadcast_transaction(&self, tx: &Transaction) { |
There was a problem hiding this comment.
Hmm, I'm not sure if this would qualify for the unsafe_ prefix and the Warning. It's really just the normal flow, just that we leave broadcasting to the user instead of using the BroadcasterInterface.
There was a problem hiding this comment.
we leave broadcasting to the user instead of using the BroadcasterInterface
actually, I think it would be good to emit an event ClientPaidSoPleaseBroadcastTransactionNow instead of doing it automatically, or even better, reuse the LdkEvent::FundingTxBroadcastSafe, which is literally the event used to let the user know they should manually broadcast a transaction. I will investigate if that's possible
lightning/src/routing/router.rs
Outdated
| @@ -1075,7 +1075,7 @@ impl PaymentParameters { | |||
| } | |||
|
|
|||
| /// A struct for configuring parameters for routing the payment. | |||
| #[derive(Clone, Copy)] | |||
| #[derive(Clone, Copy, Debug)] | |||
| @@ -35,7 +35,7 @@ lightning = { version = "0.2.0", path = "../lightning", default-features = false | |||
| lightning-macros = { version = "0.2", path = "../lightning-macros", default-features = false } | |||
| bitcoin = { version = "0.32.2", default-features = false } | |||
| futures = { version = "0.3", optional = true } | |||
| esplora-client = { version = "0.12", default-features = false, optional = true } | |||
| esplora-client = { version = "0.11", default-features = false, optional = true } | |||
There was a problem hiding this comment.
Please don't include unrelated changes here.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
from
a038ab6 to
eb5f42f
Compare
|
This needs a rebase now that #3662 landed. |
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
3 times, most recently
from
8d7da60 to
82a4bc1
Compare
I'm close, I have a half baked functional test. I want to finish that before putting this as ready for review |
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
3 times, most recently
from
fb259f4 to
6412945
Compare
|
ok, this should be ready now @tnull all comments are addressed in fixup commits. also I wrote a full end to end test that covers the client_trusts_lsp flow (directly in this repo, not in ldk-node, which was not possible before! 😄 ) . also added some documentation that explains how the client_trusts_lsp flow works. thanks! |
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
| pub fn payment_forwarded(&self, next_channel_id: ChannelId) -> Result<(), APIError> { | ||
| /// [`Event::BroadcastFundingTransaction`]: crate::lsps2::event::LSPS2ServiceEvent::BroadcastFundingTransaction | ||
| pub fn payment_forwarded( | ||
| &self, next_channel_id: ChannelId, skimmed_fee_msat: Option<u64>, |
There was a problem hiding this comment.
IMO skimmed_fee_msat shouldn't be an Option. That implies to a user that they can skip it. Its only an Option in the corresponding Event for serialization backwards compatibility, but we should push the user to simmed_fee_msat.unwrap_or(0).
| /// | ||
| /// This method should only be used when manual control over transaction | ||
| /// broadcast timing is required (e.g. client_trusts_lsp=true) | ||
| pub fn broadcast_transaction(&self, tx: &Transaction) { |
There was a problem hiding this comment.
I'm really not sure what the point of this is. We're already holding a reference to the BroadcasterInterface (now), so why are we creating an Event with the sole purpose of making the user call a method back on the code that generated the event? Why can't we just do the broadcast directly and remove the event entirely? And, if we keep the event, what's the point of having a BroadcasterInterface reference is its just used for a public method that proxies it.
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
from
7cfa0b4 to
eb77ece
Compare
|
thanks for the review @TheBlueMatt ! all fixups are squashed, and also addressed the latest comments in new fixup commits thanks!! |
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
from
eb77ece to
90ea0f8
Compare
There was a problem hiding this comment.
Fixups LGTM, I'll leave it to @tnull to look at this one last time and request remaining fixups be squashed.
There was a problem hiding this comment.
Current code mostly LGTM, some comments.
I still think we need to make sure nothing unexpected happens if we hit HTLC timeouts before the funding transaction is broadcast and confirmed. In particular, we need to make sure that a malicious client can't have incoming channels to LSP get force-closed and that the LSP is able to claim all their funds at any point in the flow.
IIRC, we previously discussed different 'hacky' ways to convince LDK to simply fail back the HTLCs (e.g., 'simulate' confirming the funding transaction and/or any force-close transaction), but the conclusion was that we want/need to create a more easy-to-use/safe interface. (cc @TheBlueMatt)
| /// were being held for that JIT channel are forwarded. In a `client_trusts_lsp` flow, once | ||
| /// the fee has been fully paid, the channel's funding transaction will be broadcasted. | ||
| /// | ||
| /// Note that `next_channel_id` and `skimmed_fee_msat` are required to be provided. Therefore, the corresponding | ||
| /// [`Event::PaymentForwarded`] events need to be generated and serialized by LDK versions | ||
| /// greater or equal to 0.0.107. |
There was a problem hiding this comment.
No, 0.0.107 is only the min required version for next_channel_id, skimmed_fee_msat was introduced with 0.0.122 to PaymentForwarded.
There was a problem hiding this comment.
I was about to rewrite this to say
/// For LDK >= 0.0.107 and < 0.0.122 pass `0` for `skimmed_fee_msat` and only use `client_trusts_lsp = false`
but that won't work since now we are also checking skimmed_fee_msat for lsp_trusts_client flows, and the flow won't progress to OutboundJITChannelState::PaymentForwarded if the fee is not paid
so I guess LDK's minimum version for LSPS2 should be 0.0.122 now so skimmed_fee_msat is always present?
/// Note that `next_channel_id` and `skimmed_fee_msat` are required to be provided. Therefore, the corresponding
/// [`Event::PaymentForwarded`] events need to be generated and serialized by LDK versions
/// greater or equal to 0.0.122.
There was a problem hiding this comment.
Yeah, it will only be possible to upgrade from 0.0.122 and above, which is perfectly fine though.
| @@ -1412,11 +1534,139 @@ where | |||
| peer_state_lock.is_prunable() == false | |||
| }); | |||
| } | |||
|
|
|||
| /// Checks if the JIT channel with the given `user_channel_id` needs manual broadcast. | |||
| /// Will be true if client_trusts_lsp is set to true | |||
There was a problem hiding this comment.
nit: Please tick:
| /// Will be true if client_trusts_lsp is set to true | |
| /// | |
| /// Will be `true` if `client_trusts_lsp` is set to `true`. |
| } | ||
|
|
||
| /// Called to store the funding transaction for a JIT channel. | ||
| /// This should be called when the funding transaction is created but before it's broadcast. |
There was a problem hiding this comment.
"but before it's broadcast" could be a bit more verbose or dropped. Is there any additional responsibility on the user, or can they just call this when they created the funding tx?
| // 7. Call the service's channel_ready function | ||
| // 8. The service will now forward the intercepted HTLC to the client on the new JIT channel | ||
| // 9. The client will see the PaymentClaimable event | ||
| // 10. Assert that the service has not broadcasted the funding transaction yet, because the client has not claimed the HTLC yet |
There was a problem hiding this comment.
Can we add a test variant that at this point waits for a long time, surpassing the incoming HTLCs timeouts? In particular, can we check that the incoming channel isn't force-closed when the client doesn't claim the payment for a long time (or is offline)?
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
from
90ea0f8 to
b99b6d3
Compare
|
thank you for the review @tnull I squashed the old fixups and pushed 2 new fixups that address the latest comments. thanks! |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Mmm, this is a good point. If an (outbound) HTLC is set to expire soon LDK will FC the channel and broadcast the commitment tx (which will not be confirm-able, as we never broadcasted the funding transaction). If the downstream logic doesn't do an anchor-output CPFP bump, that's totally fine. LDK will be unable to get the commitment tx confirmed and no problem, at least as long as If LDK does do a CPFP bump against the channel that doesn't exist that's problematic, but its not clear to me what to do about it - I don't think we want to never broadcast the commitment transaction if the funding is unconfirmed, the commitment tx may end up CPFP'ing the funding transaction in some cases and we want to allow for that (esp with anchors!). I suppose we could track the manual-broadcast flag, pass that through to the
Hmm, I vaguely recall this discussion being more about the implementation itself, which is handled with the manual-broadcasting methods added here, but I could be wrong. |
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
from
b99b6d3 to
c117dd6
Compare
|
had to rebase with main to fix a silent conflict on the lsps2 tests this was the thing missing: 
CI should pass now |
just pushed a new fixup commit (f37b129) that adds some logic and a test that asserts that the funding tx cannot be broadcast after the channel is closed.
not sure what to do about this. I can do the "track the manual-broadcast flag and pass it to the ChannelMonitor to not broadcast commitment txs" but not sure if that's what we want. thoughts? @TheBlueMatt @tnull |
|
IMO that's what we want, but I certainly don't feel strongly. @tnull? |
|
Actually, yea, I do think we should do that. I don't see a reason why that would be problematic as long as we document it carefully on the manual-broadcast method. |
|
|
||
| if let Some(funding_tx) = funding_tx { | ||
| self.tx_broadcaster.broadcast_transactions(&[&funding_tx]); | ||
| // Broadcast the funding transaction only if the LDK channel is still usable. In |
There was a problem hiding this comment.
Hmm, wouldn't checking is_usable be potentially race-y if the client manages to disconnect just after claiming? Would probably make sense to check is_channel_ready?
| other => panic!("Expected PaymentClaimable, got {:?}", other), | ||
| }; | ||
|
|
||
| client_node.inner.node.claim_funds(preimage); |
There was a problem hiding this comment.
Can we also advance the (service's) chain state so the HTLC timeout would be hit before the client claims the HTLC, and see what happens?
The feature
lightningdevkit/ldk-node#479
Currently, our LSPS2 service implementation only supports the lsp_trusts_client model, which means that "If the client does not claim the payment, and the funding transaction confirms, then the LSP will have its funds locked in a channel that was not paid for."
On a client_trusts_lsp model, the LSP will NOT broadcast the funding transaction until the client claims the payment.
The plan:
(This plan was validated and acknowledged by @tnull in private). There are differences between the plan and the implementation, but it roughly describes the approach.
LSPS2 Process & Events: These are handled the same way as before. No changes here.
When the OpenChannel event is emitted, ldk-node calls create_channel as usual. The key difference is: If client_trusts_lsp = true, after emitting the OpenChannel event, we start tracking the HTLC that our client will eventually claim.
Funding Transaction Broadcast Logic: The batch_funding_transaction_generated_intern function decides whether the funding transaction should be broadcast automatically. There are two existing funding types:
I will introduce a third type:
With this:
lsps2_service on ldk-node will now interact with lsps2_service on rust-lightning in two new key moments:
Changes:
funding_transaction_generated_manual_broadcaston channel_manager. Uses FundingType::CheckedManualBroadcast, which validates but does not automatically broadcastchannel_needs_manual_broadcast. This is used by ldk-node to know if funding_transaction_generated or funding_transaction_generated_manual_broadcast should be called when FundingGenerationReady event is triggeredstore_funding_transaction. This is used by ldk-node when the funding transaction is created. We need to store it because the broadcast of the funding transaction may be deferred.funding_tx_broadcast_safe. This is used by ldk-node when the FundingTxBroadcastSafe event is triggeredbroadcast_transaction_if_appliesfrom the lsps2/serviceLDK Node integration
In this PR lightningdevkit/ldk-node#572 on ldk-node, you can see that 2 tests are created that demonstrates the funcionality described above.
client_trusts_lsp=true
In the test, receive_via_jit_channel_manual_claim is called, the mempool is checked to assert that the funding transaction was not broadcasted yet (it should not because client_trusts_lsp=true, and the client has not claimed the htlc yet).
Then the client calls
claim_for_hash, and the mempool is checked again, and now the funding transaction should be thereclient_trusts_lsp=false
In the test, receive_via_jit_channel_manual_claim is called, the mempool is checked to assert that the funding transaction was broadcasted (because the LSP trusts the client), even though the client has not claimed the htlc yet. In this case, the LSP was tricked, and it will have its funds locked in a channel that was not paid for.
Side note: for the tests to work I had to create a new
receive_via_jit_channel_manual_claimso the client can manually claim the htlc using theclaim_for_hash.